Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace zenith with solar_zenith in variable map for pvlib:psm3 #2381

Merged
merged 3 commits into from
Feb 21, 2025

Conversation

ayushjariyal
Copy link
Contributor

@ayushjariyal ayushjariyal commented Feb 7, 2025

@cwhanse
Copy link
Member

cwhanse commented Feb 7, 2025

@ayushjariyal I feel like this bug should have been caught in the tests. Are you willing to add a check to this test to verify that all intended columns have been mapped?

@williamhobbs
Copy link
Contributor

@cwhanse, does this test also need to be updated to check for the intended columns?

def test_get_psm3_attribute_mapping(nrel_api_key):

This test already includes 'solar_zenith', but maybe I'm misunderstanding the test workflow:

def test_read_psm3_map_variables():

@cwhanse
Copy link
Member

cwhanse commented Feb 13, 2025

@cwhanse, does this test also need to be updated to check for the intended columns?

In both cases I think the better answer is no.

The error (zenith in place of solar_zenith) is in a map from pvlib names to PSM3 names. The results of that mapping are not passed to other pvlib functions; they are only checked for a correct index. Tthe test structure (for all of iotools, I think, and to a large extent all of pvlib) does not enforce pvlib's parameter nomenclature except when calling another pvlib function. That kind of enforcement strikes me as complicated, probably undesirable, and certainly, well beyond the scope of this PR.

I vote to only fix the name and thank the contributor.

@williamhobbs
Copy link
Contributor

I vote to only fix the name and thank the contributor.

@cwhanse, I agree. Thanks for the clarification.

@cwhanse cwhanse added the bug label Feb 13, 2025
@cwhanse cwhanse added this to the v0.11.3 milestone Feb 13, 2025
@cwhanse
Copy link
Member

cwhanse commented Feb 13, 2025

@ayushjariyal I don't think we need a note in the whatsnew for this fix, and you're already in the list of contributors. So this is done I think.

@RDaxini
Copy link
Contributor

RDaxini commented Feb 14, 2025

Thanks for working on this @ayushjariyal
Can we change the title of this PR to something like (suggestion) "Change zenith to solar_zenith in psm3 variable map"? Might make it easier to find if needed for future reference

@cwhanse cwhanse changed the title solve issue 2377 Replace zenith with solar_zenith in variable map for pvlib:psm3 Feb 14, 2025
@kandersolar kandersolar merged commit 171f10c into pvlib:main Feb 21, 2025
27 checks passed
@kandersolar
Copy link
Member

Thanks @ayushjariyal!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zenith vs solar_zenith in psm3 variable maps
5 participants